BackupDB 1.0 #11
BackupDB 1.0 #11
Conversation
|
If @gavinwahl @rockymeza or @davesque have any feedback, I'm all ears. |
| suffix=backup_suffix, | ||
| ext=backup_cls.extension, | ||
| ) | ||
| absolute_fname = os.path.join(backup_directory, fname) |
There was a problem hiding this comment.
Does this need to be wrapped in an os.path.abspath? It doesn't appear that get_backup_directory returns an absolute path.
There was a problem hiding this comment.
No, you're totally right. I'm just not sure where to get the absolute path. Should it be os.path.join(PROJECT_PATH, BACKUP_DIR), because BACKUP_DIRECTORY doesn't have to be an absolute directory.
There was a problem hiding this comment.
Since os.path.abspath is idempotent, why don't you just pass the output of get_backup_directory through it to ensure it's absolute.
There was a problem hiding this comment.
PROJECT_PATH is more of a Fusionbox concept, but most django installs probably have BASE_DIR.
That being said. I would much rather have the user decide that and ask that they provide absolute URLs instead of us guessing what things they have in their settings.
There was a problem hiding this comment.
You also might consider calling os.path.expanduser before calling abspath.
There was a problem hiding this comment.
@rockymeza I don't think it being relative now will matter. Is there something specific that you're concerned about expanding relative paths to absolute paths?
There was a problem hiding this comment.
All of the builtin Django settings are absolute, like MEDIA_ROOT, etc.
I'm just opposed to guessing.
There was a problem hiding this comment.
@acatton When you say guessing, are you referring to using backups/ as the default?
There was a problem hiding this comment.
abspath seems like a good idea.
But I don't think we should call os.path.expanduser. If you want to do it in your settings BACKUP_DIRECTORY = os.path.expanduser(yourpath), that's fine. What if I want my backups to go in '~backups/' in my project?
|
Cool to see this getting love. Looks like a great start. Might I suggest you look into using tox to test this across the different database backends? I think it will make your life a lot easier. |
|
Looks nice. I never really liked the "do_backup_..." function system. Having backend classes is more consistent with the design of other django libraries. |
|
@pipermerriam regarding tox, see 7548740. I'm not sure why travis didn't pick it up yet :-/ . |
Thank you Piper Merriam for the suggestion.
|
A suggestion. It looks like you have a pretty complex Since tox has facilities for doing env variable setting and everything within the individual environments, it becomes a lot easier to get all of your envs running with the right env variables, etc. I think this will make it a bit easier for people to run the full suite on their own machines. |
This is a huge refactoring of BackupDB.
This pull request is not mergable yet. This is more of a "Request For Comments":
pip install -e python-spm/.TODOnotespython manage.py backupdb --stdoutto dump the database on stdout. (The original reason I refactored it)This is supposed to: